Skip to content

feat(zkevm): add log opcode worst case #1745

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

LouisTsai-Csie
Copy link
Collaborator

@LouisTsai-Csie LouisTsai-Csie commented Jun 15, 2025

🗒️ Description

Create Benchmark for LOG Opcode

This PR introduces a benchmark for the LOG opcode in the EVM. The LOG operation includes multiple fields: offset, size, and up to N topics depending on the variant (LOG0 through LOG4).

Gas Cost Calculation

Using LOG0 as an example, the gas cost for the LOG operation is computed as:

  • Static gas: 375
  • Dynamic gas:dynamic_gas = 375 * topic_count + 8 * size + memory_expansion_cost

To minimize gas costs:

  • Use zero size to avoid dynamic gas from memory access.
  • Use PUSH0 (which costs 2 bytes) for pushing values to the stack, as it’s cheaper than other operation like PUSH1 (3 bytes with an immediate) or DUP1 (3 bytes).

Using PUSH0 is also more space-efficient, since it avoids including immediate values and reduces bytecode size.

Benchmark Bytecode Pattern

A minimal loopable pattern for benchmarking LOGN looks like this:

JUMPDEST
PUSH0      ; offset
PUSH0      ; size
...        ; PUSH0 repeated N times for each topic
LOGN
...        ; repeatable pattern
PUSH0
JUMP

Using PUSH0 is also more space-efficient, since it avoids including immediate values and reduces bytecode size. This makes it preferable over DUP1 for setting up the stack for topics or memory offset/size.

🔗 Related Issues

Issue #1689

✅ Checklist

  • All: Set appropriate labels for the changes.
  • All: Considered squashing commits to improve commit history.
  • All: Considered updating the online docs in the ./docs/ directory.
  • Tests: Included the type and version of evm t8n tool used to locally execute test cases: e.g., ref with commit hash or geth 1.13.1-stable-3f40e65.
  • Tests: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.

@LouisTsai-Csie LouisTsai-Csie self-assigned this Jun 15, 2025
@LouisTsai-Csie LouisTsai-Csie marked this pull request as ready for review June 15, 2025 14:19
@LouisTsai-Csie LouisTsai-Csie added the scope:tests Scope: Changes EL client test cases in `./tests` label Jun 16, 2025
@LouisTsai-Csie LouisTsai-Csie requested review from jochem-brouwer and jsign and removed request for jochem-brouwer June 16, 2025 09:19
Copy link
Member

@jochem-brouwer jochem-brouwer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test looks fine to me, it tests all LOG opcodes and runs those all as much as possible. However, I think we should also add these situations:

  • Log data length is nonzero
  • Log data length + data is nonzero (here we have to initialize the memory to something non-zero)
  • The topics are nonzero

I am not sure because I am not deep into zkEVM, is calculating the logsBloom part of the scope of the EVM (and would thus add extra logic/cycles) @jsign? If this is indeed part of it (either logsBloom or calculating receiptTrie or both) then those tests should be added for zkEVM (can be done in a different PR).

Those tests are also nice benchmark tests for non-zkEVM also 😄 👍

Will not explicitly approve for now.

@LouisTsai-Csie
Copy link
Collaborator Author

Thanks for comment, I do not know logsBloom and receiptTrie before, i will review some documentation for them

@jsign
Copy link
Collaborator

jsign commented Jun 16, 2025

This test looks fine to me, it tests all LOG opcodes and runs those all as much as possible. However, I think we should also add these situations:

  • Log data length is nonzero
  • Log data length + data is nonzero (here we have to initialize the memory to something non-zero)
  • The topics are nonzero

I am not sure because I am not deep into zkEVM, is calculating the logsBloom part of the scope of the EVM (and would thus add extra logic/cycles) @jsign? If this is indeed part of it (either logsBloom or calculating receiptTrie or both) then those tests should be added for zkEVM (can be done in a different PR).

Those tests are also nice benchmark tests for non-zkEVM also 😄 👍

Will not explicitly approve for now.

Yep, calculating logsBloom/receipts is part of proving -- so I agree that it is useful to cover the cases you mentioned.

Copy link
Collaborator

@jsign jsign left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Reg @jochem-brouwer suggestion to cover non-zero scenarios (which I agree are useful), I'll let you decide if those might be worked in this PR or in a separate one -- for now leaving an approval if it is the latter.

Just left one comment for your consideration.



@pytest.mark.parametrize(
"opcode,topic_count",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can remove the topic_count parameter, and derive it from opcode - Op.LOG0?
Mainly because combinations like Op.LOG0, 1 or Op.LOG3, 4 would never make sense?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this suggestion 😄

Copy link
Member

@marioevz marioevz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just one suggestion that is in line with the comments from previous reviewers, thanks!

env = Environment()
max_code_size = fork.max_code_size()

iter_code = Op.PUSH0 * (2 + topic_count) + opcode
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
iter_code = Op.PUSH0 * (2 + topic_count) + opcode
iter_code = Op.PUSH0 * len(opcode.kwargs) + opcode

And you can remove the topic_count parameter.

@LouisTsai-Csie LouisTsai-Csie marked this pull request as draft June 17, 2025 10:55
@marioevz
Copy link
Member

For the mypy error, I think if you rebase to main it should be gone, I just fixed that in another PR.

calldata = (
Op.PUSH0
if empty_value
else Op.PUSH32(0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could be further optimized for readability

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about:

Suggested change
else Op.PUSH32(0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF)
else Op.PUSH32(2**256-1)

if empty_value
else Op.PUSH32(0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF)
)
topic_count = len(opcode.kwargs or []) - 2
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I want to extract the topic count, it is kwargs - 2 because offset and value should be excluded. And I could not use len(opcode.kwargs) - 2, this would result in data type checking issue.

For kwargs, it is either a List or None type, for the latter one, there is no len method. - Link

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this is annoying, and most of the opcodes have keyword arguments already, we might need put kwargs to all opcodes and remove the | None at some point.

)
topic_count = len(opcode.kwargs or []) - 2

offset = 0 if fixed_offset else Op.MOD(Op.GAS, 7)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jsign for this randomize trick!

@LouisTsai-Csie
Copy link
Collaborator Author

@jsign @jochem-brouwer I’ve updated the test case and added the following scenarios:

  • Topics are zero and non-zero
  • Log data has a non-zero length

Could you please help me review again? And i am asking why the CI is failing

Copy link
Member

@marioevz marioevz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Just some minor comments.

I think we can move this new test function into a new file, perhaps tests/zkevm/test_worst_opcode.py ?

We can start by moving only this new function for now in this PR, I have an idea on how to fix the coverage script and we can move the rest once that's fixed.

if empty_value
else Op.PUSH32(0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF)
)
topic_count = len(opcode.kwargs or []) - 2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this is annoying, and most of the opcodes have keyword arguments already, we might need put kwargs to all opcodes and remove the | None at some point.

calldata = (
Op.PUSH0
if empty_value
else Op.PUSH32(0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about:

Suggested change
else Op.PUSH32(0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF)
else Op.PUSH32(2**256-1)

Comment on lines 2191 to 2276
0, # 0 bytes
10, # 10 bytes
100, # 100 bytes
1 * 1024, # 1KiB
10 * 1024, # 10KiB
100 * 1024, # 100KiB
1024 * 1024, # 1MiB
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel this yields too many tests, we should reduce it maybe to zero memory and 1MiB, so it's clear during benchmarking whether the size affects performance or not.

pre: Alloc,
fork: Fork,
opcode: Opcode,
empty_value: bool,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
empty_value: bool,
empty_topic bool,

empty_value reads a bit like value in the transaction.

@LouisTsai-Csie
Copy link
Collaborator Author

@marioevz Thanks for review. My original problem for evmone coverage report is that the CI never ends, but after I rebase again, the issue changed.

After checking the coverage report, the root cause is from BLS signature, which has nothing to do with this benchmark. I move the test case to tests/zkevm/test_worst_opcode.py, let's see if this fix the CI issue

@jsign @jochem-brouwer could you please also review this benchmark, let me know if I miss any cases!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature:benchmark feature:zkevm scope:tests Scope: Changes EL client test cases in `./tests`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants